Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dealing with createMsgChannel errors #35

Merged

Conversation

film42
Copy link
Collaborator

@film42 film42 commented Nov 7, 2017

I'm taking a stab at #32 but can't be sure if this fixes it just yet.

Here's what I'm trying:

  1. Make the start_client_nats_connection avoid setting the memo until the client connection has completed.
  2. Check @start_client_nats_connection && @client_nats_connection when memo-ing the client nats connection setup. I'm not sure this is actually addressing anything because in the examples we were clearly inside an instance of JNats but only the inner @connection was nil.
  3. Retry connecting with the same options provided to #connect should we ever find the @connection nil. I'm not sure this is the best idea, but if we get "no servers found" errors we'll know there's a race condition in the setup code. I added a test here but I'm not sure we'll want to keep this behavior forever.

NOTE: This is pointed at @abrandoned 's pre-released branch right now.

cc @abrandoned @quixoten

@film42
Copy link
Collaborator Author

film42 commented Nov 7, 2017

Rebased and ready for 👀 .

# Do not depend on #close for a greaceful disconnect.
def connection
return @connection unless @connection.nil?
connect(options || {})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this path call close? (or at the top of connect?)

@abrandoned
Copy link
Owner

:shipit:

@abrandoned abrandoned merged commit 0c724d2 into abrandoned/nats_subscription_pooling Nov 7, 2017
@film42 film42 deleted the gt/whats_up_with_chans branch November 7, 2017 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants